-
Notifications
You must be signed in to change notification settings - Fork 39
Improve Container Types #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
if (aSize > Capacity()) | ||
Reserve(aSize); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (aSize > Capacity()) | |
Reserve(aSize); | |
if (aSize > Capacity()) | |
{ | |
Reserve(aSize); | |
} |
- Fixed issues - Added more methods (Erase, Insert, etc.) - Refactored various methods - Improved semantics - Generally cleaned up
Id still like to have a look if I may |
|
||
DynArray& operator=(DynArray&& aOther) noexcept | ||
{ | ||
if (this != std::addressof(aOther)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply swap contents for moving ops btw? Should be cleaner and shorter code overall. Also no need to check for this etc. in such instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it was @wopss who wanted it this way, as it doesn't introduce any complexity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember complaining about this. I also don't get why we don't do std::swap
.
} | ||
|
||
auto distance = std::distance(aFirst, aLast); | ||
DifferenceType newSize = std::abs(distance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont like allowing negative values, shouldnt be the case for proper range. Only case this should ever return negative is for random access range where you swapped first and last, shouldnt affect reverse iterators on the range AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im actually unsure if we should even care for distance, IMO for generic case like this when iterators/range isnt random access, it should just use back_inserter, this may introduce tons of unwanted overhead. Iteration over some types isnt fast, and iterating through it twice unnecessarily may bottleneck things unless it is random access range as mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you suggest I avoid the double evaluation?
I assume it involves changing the template concept to std::forward_iterator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, use std::back_inserter instead of directly using begin/end for copying. And preallocate space only when you have std::random_access_iterator via reserving and not resizing. For std::random_access_iterator, std::distance is constant operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would std::back_inserter
work like you suggest? DynArray doesn't currently (STL semantically) define push_back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldnt that is true, there should be push_back also in the STL portion of the containers, along with value_type typedef so you can use std::ranges::value_type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I believe we already do have an overloaded operator[] right? That is sort of a key for random_access_iterator to be valid, all iterators that are for random access containers should also have an operator+ for that.
} | ||
|
||
template<std::input_iterator InputIt> | ||
void Assign(InputIt aFirst, InputIt aLast) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im missing some way to assign range directly, we are long past the age of having to use iterators unless absolutely necessary. Can achieve practically everything with ranges and views more cleanly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applies to constructors also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing some move-assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you suggesting here? Should I create a new method to accept a range, or adjust this one? If it's the former, what structures are implied by ranges and views?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New method ideally which accepts any range as a type. Ranges have to have begin() and end() defined, other functions like empty() and size() are not guaranteed unless you go to specializations of ranges.
It likely would end up as a wrapper for iterator-based assignments.
|
||
void PushBack(ConstReference aItem) | ||
{ | ||
EmplaceBack(std::forward<ConstReference>(aItem)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think std::forward is correct in this instance. Likely wont cause an issue but that is more thanks to its definition, shouldnt be ever needed when you know you are passing lvalue reference.
|
||
void PushBack(ValueType&& aItem) | ||
{ | ||
EmplaceBack(std::forward<ValueType&&>(aItem)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This almost certainly seems wrong, forwarding && will always result in rvalue reference even when it formerly wasnt. Just ValueType should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't even see the need of std::forward
here, a simple std::move
should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not the same, && also eats const &, when you move instead of forward, that info is lost and leads to undefined behaviour. In this instance, it should be forward as you forward the argument further down the pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue is primarily wrong function overload may get called in an instance I tried to make an example of if you use wrong one.
{ | ||
assert(aPos < End() && Includes(aPos)); | ||
|
||
aPos->~ValueType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::destroy_at
|
||
[[nodiscard]] constexpr SizeType MaxSize() const noexcept | ||
{ | ||
#undef max // avoid conflict with minmax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldnt be needed provided NOMINMAX macro is correctly defined. And if you are worrying about this, rather wrap min and max keywords in parenthesis eg. std::numeric_limits<SizeType>::(max)()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be against providing NOMINMAX
macro since others might not like it when this library is used in their code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Believe it is pretty universal tbh, and you already do WIN32_LEAN_AND_MEAN I believe but would have to double-check. Unless in C, those macros are pointless and just cause issues like the above to appear. Macro is kept only for backwards compar am fairly certain and safe to provide. Always can be undefed by user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Id be for this still.
template<std::input_iterator InputIt> | ||
StaticArray(InputIt aFirst, InputIt aLast) | ||
{ | ||
if (std::distance(aFirst, aLast) > MaxSize()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here you dont check the size and make it absolute and whatnot. Just as a note to the other distance comment I had, negative values are practically never a good sign.
include/RED4ext/DynArray.hpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought about this, but removing this header and class will cause a lot of issues. So, I propose to add it back, deprecate them (the header and the class).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of issues? The changes already include updates to all the DynArray file paths, and Reflection-inl.hpp
was updated accordingly for the dumper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People project's still use this path, changing it like that will be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplest solution to this - just have file in original location including the file from right location.
Isnt likely as clean, but should be more than sufficient imo not @wopss ?
Also the least amount of additional changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. And add a pragma message
in the header saying that it is deprecated.
- Added range based container assignment and construction - Replaced `=` operator to use `std::swap` of members - Improved semantics - Deprecated `RED4ext/DynArray.hpp` with pragma message warning
- STL compatibility - Swap method - Removed negative range handling
No description provided.